Don't link jvm on Android
#254
Conversation
Package.swift
Outdated
| @@ -209,8 +209,7 @@ let package = Package( | |||
| .when(platforms: [.windows])), | |||
| .linkedLibrary( | |||
| "jvm", | |||
| .when(platforms: [.linux, .macOS, .windows]) | |||
| ), | |||
| .when(platforms: [.iOS, .macOS, .tvOS, .watchOS, .macCatalyst, .linux, .openbsd, .wasi, .windows])), | |||
There was a problem hiding this comment.
We’re not really promising compatibility with a bunch of those yet
There was a problem hiding this comment.
I'd suggest removing: wasi, wasm, catalyst, ios, tvos, watchos -- it's not platforms we support right now
| .when(platforms: [.iOS, .macOS, .tvOS, .watchOS, .macCatalyst, .linux, .openbsd, .wasi, .windows])), | |
| .when(platforms: [.macOS, .linux,])), |
wdyt?
| @@ -30,7 +30,7 @@ func findJavaHome() -> String { | |||
| } | |||
| let javaHome = findJavaHome() | |||
|
|
|||
| let javaIncludePath = "\(javaHome)/include" | |||
| let javaIncludePath = ProcessInfo.processInfo.environment["JAVA_INCLUDE_PATH"] ?? "\(javaHome)/include" | |||
There was a problem hiding this comment.
Worth calling out in the readme? What do the paths look like for android?
There was a problem hiding this comment.
@lhoward I cherry picked your commit, can you comment please?
There was a problem hiding this comment.
It's a while since I looked at this. JAVA_INCLUDE_PATH is something I used in swift-build-android.sh, I'm not sure if it's a standard environment variable or not.
There was a problem hiding this comment.
Probably not standard but my question was more about "why do we need this" / "why are the paths different and how do they look like" and just adding a note to the README about how to use this
There was a problem hiding this comment.
We need to pick up the host JVM when building Java2Swift and JavaCompilerPlugin, vs the Android JVM when building JavaKitExample. JAVA_HOME and JAVA_INCLUDE_PATH serve to disambiguate between the two.
There was a problem hiding this comment.
Sounds good, can we plop such a note in to the readme somewhere along with this pr please?
| @@ -27,7 +27,10 @@ extension HelloSwift: HelloSwiftNativeMethods { | |||
| let answer = self.sayHelloBack(i + j) | |||
| print("Swift got back \(answer) from Java") | |||
|
|
|||
| #if !canImport(Android) | |||
| // no class variables in Kotlin | |||
There was a problem hiding this comment.
But Android != Kotlin, there's Java there too right? the comment strikes me as bit weird, is this really necessary?
There was a problem hiding this comment.
The sample Android app is written in Kotlin. I do not have the cycles to rewrite it in Java.
There was a problem hiding this comment.
Maybe I'm confused, this is the JavaKitSampleApp, why does what the android sample app matter to this example project?
| public let environment: JNIEnvironment | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I'm not sure about such a semantic change based on platform... can would this actually do the right thing or lead to unexpected issues sometimes?
There was a problem hiding this comment.
Please see the comment in commit a2ba0e16d6255abb62598910151056f54c92340e.
There was a problem hiding this comment.
JavaKit: always use ambient javaEnvironment on Android
This is not a real fix, I suspect the real fix is to remove javaEnvironment as
an instance variable on all platforms and always use the ambient environment
(as the JNI specification clearly states the environment cannot be shared
between threads).
Works around: #157
|
Added some more questions, with the C++ file I'm ok with I guess. We should think about Android CI eventually... |
| return (*JavaRuntime_GetCreatedJavaVMs)(vmBuf, bufLen, nVMs); | ||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
This code needs to be removed and replaced with targeting a NDK version which exports the JNI.
There was a problem hiding this comment.
It should be fairly trivial to do with a current Swift Android SDK, I just haven't had time to do it.
|
@colemancda can we step back a little and clarify the PR a bit? It says we "don't link jvm on android" which is fine, and I'd be happy to merge that, but then there's also a bunch of picked fly-by changes that don't seem fully fleshed out. How about we make this PR the minimum change to not link the jvm and the rest let's do in separate follow ups? |
I'm confused how these relate to the Android PRs I filed, if at all? |
|
Bottom line is: I don't know what to do about this PR specifically because it picks a bunch of stuff I'm not entirely clear about. It doesn't seem like something we should accept given the bunch of comments on the commits other than the first one. Should we just make this PR the first commit and the others later on? |
colemancda
force-pushed
the
feature/android
branch
from
4dc10f0 to
eb86790
Compare
|
@ktoso I force pushed so the PR is only 1 commit. I will clean up the other work and open a separate PR. |
|
Sounds good, thank you! We'll also work towards getting Android CI in the near future but it depends on Swift getting official android support on swift.org AFAICS. Thanks for your work! More than happy to keep accepting Android PRs but I may be missing some context since I've not looked at it much yet, thank you in advance! |
Android doesn't have an equivalent
jvmso this library should not be linked for that platform.